Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[uss_qualifier] Switch default ASTM NETRID to v22a and reorganize CI tests #119

Merged
merged 13 commits into from
Jul 12, 2023

Conversation

barroco
Copy link
Contributor

@barroco barroco commented Jun 23, 2023

This PR switches the default version of ASTM NETRID to v22a and updates the local_test to run the uspace test suite. In addition, the CI job will be split in two steps to cover U-Space Test Suite as well as the NETRID v19 separately. v19 has been preserved in order to exercise supporting multiple versions of the standard.
The github actions job and uss_qualifier manifests have been updated to run v19 tests and, U-Space related tests with properly configured mock_uss (v22a).

Please note that the issue reported in #28 seems to be due to the atproxy. I would like to suggest to disable it for few days in the CI to see if the 999 timeout still occurs. If not, that would demonstrate that we isolated the problem. This change is proposed in this PR and the atproxy client is replaced by an actual mock_uss when applicable. Please let me know if you would like to approach this differently. -> move to an upcoming PR.

@barroco barroco force-pushed the ci-v22a branch 3 times, most recently from d551b6d to 77ff647 Compare July 7, 2023 16:30
@barroco barroco changed the title [uss_qualifier] Add rid mock v22a and switch local_test default rid to v22a [uss_qualifier] Add rid U-Space test suite and ASTM NETRID v19 to CI Jul 7, 2023
@barroco barroco marked this pull request as ready for review July 7, 2023 21:39
Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice to have all that running properly together :)

IMO all comments are nits, except for the separate PR disabling the atproxy.

In addition to review comments, some nits:

  • in monitoring/mock_uss, maybe rename run_locally_riddp.shto run_locally_riddp_v19.sh for clarity? (same respectively for run_locally_ridsp.sh)
  • In the CI/local_test, rather than having the uspace test ran, having the individual suites ran separately could be nice (would help identify in GH Actions directly the failures). But that's not really the subject of this PR.

.github/workflows/CI.md Outdated Show resolved Hide resolved
monitoring/mock_uss/start_all_local_mocks.sh Outdated Show resolved Hide resolved
monitoring/mock_uss/stop_all_local_mocks.sh Show resolved Hide resolved
@@ -0,0 +1,43 @@
name: Local tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this definition really belong here? My understanding is that in suites/ we have "low-level" / base test definitions, that we can instrument from definitions in configurations/. So I have the feeling this would belong in configurations/dev, it does not feel like a dev categorty belong here. (I'm not 100% confident I'm right on this though)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dev suite was already existing for local_test to create dedicated suites which include test data generation. Based on your comment and based on a discussion with @BenjaminPelletier few weeks ago, I took the opportunity to switch the repo defaults to RID v22a and updated the local_test to run the uspace suite to cover as many as possible tests. v19 stays as an isolated test. Please let me know what you think.

@barroco barroco marked this pull request as draft July 12, 2023 09:21
@barroco barroco changed the title [uss_qualifier] Add rid U-Space test suite and ASTM NETRID v19 to CI [uss_qualifier] Switch default ASTM NETRID to v22a and reorganize CI tests Jul 12, 2023
Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made an additional pass, LGTM!

@barroco barroco marked this pull request as ready for review July 12, 2023 12:05
@barroco barroco merged commit e4828d6 into interuss:main Jul 12, 2023
@barroco barroco deleted the ci-v22a branch July 12, 2023 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants